Unimplement Send/Sync for ::env::{Args,ArgsOs,Vars,VarsOs} #48005
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
Are we certain that all current platforms explicitly opt-out / don't implement Send/Sync? If so, then this seems fine to me. Please change the annotations to stable, though, as there's no way to gate trait impls today. |
panicbit
force-pushed
the
env_unimpl_send_sync
branch
from
514e2a6
to
b439632
Compare
|
Ok, I marked the impls as stable. Here's an overview over the Send/Sync impls: Types not implementing Send/Sync:Types implementing Send/Sync: |
|
So this is a breaking change for Windows and CloudABI host compilers... seems like we're going to potentially end up breaking some users here. This should be detectable with a crater run though, so let's get the artifacts for that: @bors try. Meanwhile, @rust-lang/libs, what do you think about this change? |
|
Can we not make these types Send and Sync on all platforms?
…On Sun, Feb 4, 2018 at 3:14 PM Mark Simulacrum ***@***.***> wrote:
So this is a breaking change for windows::Vars and cloudabi in general...
seems like we're going to potentially end up breaking some users here. This
should be detectable with a crater run though, so let's get the artifacts
for that: @bors <https://github.com/bors> try. Meanwhile, @rust-lang/libs
<https://github.com/orgs/rust-lang/teams/libs>, what do you think about
this change?
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#48005 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABY2UVJWeFMGJ45erjBl7c0-4e5nRRHcks5tRjnBgaJpZM4R4s49>
.
|
|
@panicbit are you sure that there's any platform where these types are |
|
@alexcrichton Right, Windows' Vars impl actually contains pointers. I'm not sure wether it's fine to have them impl Send + Sync. |
|
Yes It's currently intentionally conservative that they aren't send/sync I believe, but I don't believe this is a breaking change as mentioned here? |
pietroalbini
added
T-libs-api
|
@bors try Please don't include any punctuations when commanding bors, they don't like that 😉. |
Unimplement Send/Sync for ::env::{Args,ArgsOs,Vars,VarsOs}
Fixes #48004
|
☀️ Test successful - status-travis |
|
@panicbit just to confirm, you still think this is a breaking change, right? (I'm still not so sure myself) |
|
@alexcrichton I never expressed the thought of this being a breaking change. I don't think it's really breaking as the current state of things is already broken / unspecified. Though, I'm unsure wether it's best to force If someone familiar with the Windows internals could confirm that the Windows types can be Send/Sync (without adding locks), I'd lean towards making everything Send/Sync. |
|
Ah ok sorry I think I misunderstood. Perhaps a test could be added to assert these aren't sync/send and then we're good to go? I can confirm the windows types are not send/sync, and we can add in in later if we want. |
|
@alexcrichton @panicbit Looks like this is waiting on further investigation for Windows, and not waiting on crater? |
|
@BatmanAoD oh nah I was originally thinking we'd want a test for this, but on second thought I think this is good to go @bors: r+ |
|
📌 Commit b439632 has been approved by |
bors
added
S-waiting-on-bors
…excrichton
Unimplement Send/Sync for ::env::{Args,ArgsOs,Vars,VarsOs}
Fixes rust-lang#48004
Correct a few stability attributes * `const_indexing` language feature was stabilized in 1.26.0 by #46882 * `Display` impls for `PanicInfo` and `Location` were stabilized in 1.26.0 by #47687 * `TrustedLen` is still unstable so its impls should be as well even though `RangeInclusive` was stabilized by #47813 * `!Send` and `!Sync` for `Args` and `ArgsOs` were stabilized in 1.26.0 by #48005 * `EscapeDefault` has been stable since 1.0.0 so should continue to show that even though it was moved to core in #48735 This could be backported to beta like #49612
|
Is this comment still true? For cli arguments, both unix and windows are defining pub struct Args {
args: vec::IntoIter<OsString>,
}which seems to be I am not sure about that cliabi platform as I didn't find it in the source code... |
Fixes #48004